-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove Hardcoded Schema References #240
Conversation
Signed-off-by: Sakshi Bobade <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good long term solution.
src/services/ResolveRef.ts
Outdated
|
||
const resolveRef = (ref: string) => { | ||
const refPath = ref.split('#')[1]; | ||
let result = serverCommon.$defs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we are resolving only server-common references? I do not know how likely we will have references to other schemas in future but we'd better handle it at least through log / warnings if some refs were not resolved.
Also how are internal refs handled now, like "$ref": "#/$defs/pkcs12-certificate"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it would be better to do
const refParts = ref.split('#');
const refFile = refParts[0];
const refAnchor = refParts[1];
if (!refFile) { refFile = zowe-yaml-schema.json }
else { // gather the $id values of each schema file and make a map so you can identify which schema file is being referenced }
src/services/ResolveRef.ts
Outdated
|
||
// Traverse and resolve references in schemas other than zowe-yaml schema | ||
Object.values(schemaMap).forEach((schema: any) => { | ||
if(schema !== schemaObject) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an invalid comparison of objects, this will always return true as objects are equal only when both are just pointers to the same object. Luckily we do not have to compare objects here, we can compare schema id's like schema?.$id !== schemaObject?.$id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it. Thank you.
src/actions/InstallationHandler.ts
Outdated
@@ -156,15 +156,8 @@ class Installation { | |||
try { | |||
let yamlSchema = JSON.parse(readPaxYamlAndSchema.details.yamlSchema); | |||
const serverCommon = JSON.parse(readPaxYamlAndSchema.details.serverCommon); | |||
updateSchemaReferences(readPaxYamlAndSchema.details, yamlSchema); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First argument to that function looks odd. You are passing {yaml, yaml-schema and server-schema} as the first argument, then you provide the yaml-schema again as the second argument as a base schema, and then you do a number of checks inside of the func and throwing an error for yaml Error parsing schema for key yaml: Unexpected token '#', "##########"... is not valid JSON
.
You have control over the readExampleYamlAndSchema
function in the same file. It returns the config and schemas altogether for simplicity, but now when we have different usage for these we can split it in two functions, or at least change returning object to something like
{..., details: {yaml: '', schemas: {<schema1.$id>: schema1, <schema2.$id>: schema2}}
and then you can simplify the updateSchemaReferences
as objects will be filtered and the schemaMap will be prepared already.
Also the readExampleYamlAndSchema
now returns only two hardcoded schemas, so we will need to update it anyway and take other schemas, but probably we can handle all of above as a separate PR, just need to add an issue to track it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @skurnevich
I would prefer passing schemas as an object containing all the available schemas, rather than using a map like this:
{ ..., details: { yaml: '', schemas: { schema1, schema2 } }}
The reason is that if we use a map e.g.:
{ ..., details: { yaml: '', schemas: { [yamlSchema.$id]: yamlSchema, [serverCommon.$id]: serverCommon } }}
We would need to maintain a strict sequence to access these schemas because we wouldn't know which schema corresponds to which $id in the code.
const schemaIds = Object.keys(readPaxYamlAndSchema.details.schemas);
// Access schemas using their ids
const yamlSchema = readPaxYamlAndSchema.details.schemas[schemaIds[0]];
const serverCommon = readPaxYamlAndSchema.details.schemas[schemaIds[1]];
This would unnecessarily complicate the code and make it harder to manage.
Does that sound good to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Proposed changes
This PR addresses Issue:
This PR refactors the code to eliminate hardcoded values for setting schema references ("ref"). By dynamically setting these references, the code becomes more flexible and less prone to errors when schemas are modified.
Type of change
Please delete options that are not relevant.
PR Checklist
Please delete options that are not relevant.
Testing
Further comments